Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net: restore TcpStream::{poll_read_ready, poll_write_ready} #2743

Merged
merged 5 commits into from
Nov 16, 2020
Merged

net: restore TcpStream::{poll_read_ready, poll_write_ready} #2743

merged 5 commits into from
Nov 16, 2020

Conversation

masnagam
Copy link
Contributor

@masnagam masnagam commented Aug 7, 2020

Motivation

In some situations, it's necessary to detect TCP disconnect without reading or writing anything to/from that socket.

This PR restores TcpStream::{poll_read_ready, poll_write_ready} which were removed at 6d8cc4e.

Solution

Restore TcpStream::{poll_read_ready, poll_write_ready}.

New test cases for those functions are added. See tcp_ready.rs.

This PR closes #2228.

@Darksonn Darksonn added A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-net Module: tokio/net labels Aug 7, 2020
@carllerche
Copy link
Member

cc @seanmonstar

@seanmonstar
Copy link
Member

Thanks for the PR!

As described in #2728, we want to remove the public dependency on mio from the API. That issue describes a proposal around how to expose readiness for socket types. The internals are changing in #2828. Once that lands, would you be interested in trying to implement what's proposed in #2728?

@masnagam
Copy link
Contributor Author

masnagam commented Sep 24, 2020

@seanmonstar I created a new working branch for that.

I implemented TcpStream::is_readable and TcpStream::is_writable for testing purposes (I'll implement correct methods later). These methods simply call PollEvented::poll_xxx_ready in poll_fn like what TcpStream::connect_addr does.

Unfortunately, tests fail, which passed before the rebase.

Probably, there are some mistakes in my changes.
Could you point out mistakes if you find?

@masnagam
Copy link
Contributor Author

#3130 was merged. Should we close this PR, or continue implementing poll_[read|write]_ready for poll_[read|write]?

@Darksonn
Copy link
Contributor

We might just have an poll_ready that takes an Interest? What do you think @carllerche?

@carllerche
Copy link
Member

I don't think we can have poll_ready. poll_read_ready and poll_write_ready makes it easier to explain the contract. There can only be one task waiting on each fn. The async fn version doesn't have this limitation because of how it is implemented.

@carllerche
Copy link
Member

I think the various socket types can still provide poll_read_ready and poll_write_ready even though there are async fn variants. Not everyone can use those (i.e. future impls).

@masnagam
Copy link
Contributor Author

PR has been rebased onto the current master branch.

TcpStream::poll_[read|write]_ready have been reimplemented using Registration::poll_[read|write]_ready.

Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-net Module: tokio/net
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detect TCP disconnect without reading or writing anything to that socket?
4 participants